Skip to content

Fix serializing special characters in Jsonite#5124

Merged
nohwnd merged 2 commits intomainfrom
jsonite
Feb 25, 2025
Merged

Fix serializing special characters in Jsonite#5124
nohwnd merged 2 commits intomainfrom
jsonite

Conversation

@nohwnd
Copy link
Member

@nohwnd nohwnd commented Feb 25, 2025

Fix #5120

Evangelink
Evangelink previously approved these changes Feb 25, 2025
Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! It might be interesting to use a DynamicData source instead of the foreach so we can replay easily but not blocking on it.

Youssef1313
Youssef1313 previously approved these changes Feb 25, 2025
Copy link
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

…n/Jsonite/Jsonite.cs

Co-authored-by: Youssef Victor <youssefvictor00@gmail.com>
@nohwnd nohwnd dismissed stale reviews from Youssef1313 and Evangelink via dffa522 February 25, 2025 12:28
@nohwnd
Copy link
Member Author

nohwnd commented Feb 25, 2025

LGTM! It might be interesting to use a DynamicData source instead of the foreach so we can replay easily but not blocking on it.

This is addressed in comments. I did not want to emit the data, because then too many serializers need to handle them correctly to get the results. If any of them is broken (dynamic data serializer or the protocol serializer) we get no info about the test in UI, leaving us blind.

// This could be converted to Data source, but this way we have more control about where in the result message the
// special characters will be (hopefully nowhere) so in case of failure, we can still serialize the message to IDE
// even if the serializer does not support special characters.

@nohwnd
Copy link
Member Author

nohwnd commented Feb 25, 2025

Tests passed, new commit is adding just comment.

@nohwnd nohwnd merged commit 06c96c2 into main Feb 25, 2025
1 of 8 checks passed
@nohwnd nohwnd deleted the jsonite branch February 25, 2025 12:36
@Evangelink
Copy link
Member

LGTM! It might be interesting to use a DynamicData source instead of the foreach so we can replay easily but not blocking on it.

This is addressed in comments. I did not want to emit the data, because then too many serializers need to handle them correctly to get the results. If any of them is broken (dynamic data serializer or the protocol serializer) we get no info about the test in UI, leaving us blind.

// This could be converted to Data source, but this way we have more control about where in the result message the // special characters will be (hopefully nowhere) so in case of failure, we can still serialize the message to IDE // even if the serializer does not support special characters.

Didn't think about it but makes total sense! Thanks!

@Evangelink
Copy link
Member

/backport to rel/3.8

@github-actions
Copy link
Contributor

Started backporting to rel/3.8: https://github.com/microsoft/testfx/actions/runs/13521711902

@microsoft-github-policy-service microsoft-github-policy-service bot added the Area: MTP Belongs to the Microsoft.Testing.Platform core library label Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: MTP Belongs to the Microsoft.Testing.Platform core library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Special characters break Jsonite

3 participants

Comments